-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not try to refresh threads to compute resume/suspend #837
Conversation
@jonahgraham This has become an issue with threads() not answering (and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the idea you have here, but I think it will lead to some display errors on targets that do support querying threads when running.
I think the correct approach is to fix:
@Override
public void thread(ThreadEventArguments args) {
refreshThreads.set(true);
fireChangeEvent(DebugEvent.CONTENT);
}
to update the list of theads
based on the information in ThreadEventArguments
and not set refreshThreads
. The doing the full refresh on every thread
event was really a shortcut and not what I assume that VSCode does.
(BTW I reviewed this quite quickly as I need to step away from my desk - I can review and run it up more fully later if the above seems like nonesense or if you are seeing something else)
You're probably right, I will try to implement as you suggest and see if it works. |
0814548
to
46f5fa4
Compare
@jonahgraham This implements your proposal and seems to work fine for vscode-js-debug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turns out this breaks a common use case and makes me realize there was always a bug there.
For single thread DAPs it is common to not send thread events at all as the one thread is created at configurationDone. The pre-existing code would call "threads" at least once.
The VSCode mock debug adapter works like this, and as mock debug is the basis for many authors of debug adapters I expect other adapters to face this issue.
The rest is the details about a bug that was always there.
The bug is that the threads call would never be sent after the initial one, unless a thread event was received. This is incorrect as the client must send a threads event after each stopped event received (from https://microsoft.github.io/debug-adapter-protocol/overview):
Supporting threads
Whenever the generic debugger receives a stopped or a thread event, the development tool requests all threads that exist at that point in time. Thread events are optional, but a debug adapter can send them to force the development tool to update the threads UI dynamically even when not in a stopped state. If a debug adapter decides not to emit Thread events, the thread UI in the development tool will only update if a stopped event is received.
After a successful launch or attach, the development tool requests the baseline of currently existing threads with the threads request and then starts to listen for thread events to detect new or terminated threads. Even if a debug adapter does not support multiple threads, it must implement the threads request and return a single (dummy) thread. The thread id must be used in all requests which refer to a thread, e.g. stacktrace, pause, continue, next, stepIn, and stepOut.
Basically the existing code supports two of the three cases enumerated in the above block, those cases being:
- Single thread (or single synthetic thread)
- Multi-threads with thread events
- Multi-threads with no thread events <-- this is the missing one
The above is relevant to your case as the adding you made in threads is partially correct, but should not remove the existing code around refreshThreads - but it should not set refreshThreads in the thread event, but should set in the stopped event.
I don't think you should be calling threads in the thread event.
(We need automated tests as that would have immediately identified the probable regression here - I will talk to Renesas to see if I can get some funded time to work on that.) |
46f5fa4
to
025877e
Compare
OK, here is another attempt which I believe matches the spec (query threads on stopped or on threads event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better, and I have re-read the spec and AFAICT this does implement the spec, but in practice debug adapters don't need to send an initial stopped or thread event to kick things off. VSCode always follows "configurationDone" with a "threads" - which means the debug adapter doesn't need to provide a stopped or thread event to get its initial state correct.
I am concerned over the removal of the synchronization in this code. IIUC the update you have here the contents of thenAcceptAsync
(in triggerUpdateThreads) may be running in multiple threads simultaneously as triggerUpdateThreads may be called multiple times in a row before the async part of previous calls has finished.
org.eclipse.lsp4e.debug/src/org/eclipse/lsp4e/debug/debugmodel/DSPDebugTarget.java
Outdated
Show resolved
Hide resolved
@@ -632,8 +587,24 @@ public void exited(ExitedEventArguments args) { | |||
|
|||
@Override | |||
public void thread(ThreadEventArguments args) { | |||
refreshThreads.set(true); | |||
fireChangeEvent(DebugEvent.CONTENT); | |||
triggerUpdateThreads(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in case where multiple threads are started in quick succession this will cause multiple threads() calls to be made in quick succession.
org.eclipse.lsp4e.debug/src/org/eclipse/lsp4e/debug/debugmodel/DSPDebugTarget.java
Outdated
Show resolved
Hide resolved
025877e
to
0b00920
Compare
New patch set re-adds synchronization and implements your 2 suggestions/fixes. For the multiple request to threads() in case multiple notification are sent; I'm not sure what would be the best solution. |
No. |
This looks ok to me, just this comment unresolved:
The current version won't display threads correctly for such debug adapters, but they will work in VSCode. |
And avoid calling stacktrace when not suspended (some LS such as vscode-js-debug may not answer in such case)
0b00920
to
b22eaad
Compare
OK, I've added an instruction to trigger a threads update upon initialization. |
Thank you very much for you review @jonahgraham ! |
Those operations typically work on the list of known threads, there is no need to refresh the threads at that time, and in case DAP's threads() request doesn't run well on non-suspended targets, then the former implementation could lead to bogus states.